Jetpack Setup: Update connection step to use APIs where possible #15983
Conversation
itsmeichigo
added
type: task
|
|
| /// Jetpack setup will fail anyway without admin role, so check that first. | ||
| let roles = stores.sessionManager.defaultRoles | ||
| guard roles.contains(.administrator) else { | ||
| throw JetpackCheckError.missingPermission | ||
| } |
There was a problem hiding this comment.
This check is redundant because fetchJetpackConnectionData will throw permission error if needed. This error is thrown if the user role is shop manager and the site is not registered with WPCom yet.
RafaelKayumov
left a comment
RafaelKayumov
left a comment
There was a problem hiding this comment.
LGTM. Followed the testing plan including the edge case with WP com connecting through shipping plugin. Works as described.
I'm still not too fluent with the Jetpack connection flow described in pe5sF9-401-p2. Code-wise looks good.
Left just 1 nit. Also a couple times I saw that the app was returning to the Login screen after entering store login/password and triggering login. Wasn't able to reproduce it on purpose.
| public init(userId: Int64, scope: String, secret: String) { | ||
| self.userId = userId | ||
| self.scope = scope | ||
| self.secret = secret | ||
| } |
There was a problem hiding this comment.
Should we rely on the synthesized initializer in this case? Looks like the manual init does the same thing?
There was a problem hiding this comment.
I had to explicitly add this initializer as I need it in JetpackSetupViewModelTests on the UI layer. The initializer needs to be public to be accessed from outside of the Networking module.
I can see this as well. The reason behind this is the failure to generate application password even though login/password authentication was successful. This happens quite arbitrarily from my experience, I'm unsure if it's only an issue with Jurassic Ninja sites. Anyway, it's not relevant to the changes in this PR I believe. Updated: I managed to consistently reproduce the issue:
I logged this issue in WOOMOB-1018. Merging this PR for now. |
| guard let wpcomCredentials, case .wpcom = wpcomCredentials else { | ||
| /// WPCom credentials are necessary to finalize connection through API | ||
| /// If this is unavailable, fall back to the web flow. | ||
| return startConnectionWithWebView() | ||
| } |
There was a problem hiding this comment.
@itsmeichigo just a question about this part and the comment there, is there a case where we might reach this screen without WPCom credentials?
On Android, we don't have any check like this, and I looked into the iOS code, and it seems to me the only place where we pass an Optional value for the credentials is here, where it seems the credentials would be necessarly present.
WDYT about this? If I'm right in my above remark, should we try to make the field in the ViewModel not optional to avoid any confusing because of this above check and comment?
There was a problem hiding this comment.
Thanks for the suggestion @hichamboushaba. I added the change in #16001.
Part of WOOMOB-965
Description
This PR updates the connection steps of the Jetpack setup flow to use the connection API following the guides in pe5sF9-401-p2.
Most of the line changes come from the unit tests. Tracking will be updated in a separate PR.
Testing steps
Follow test plan in pe5sF9-42S-p2.
Testing edge case with JCP site
Testing information
isRegisteredin the response ofGET jetpack/v4/connection/datainstead.Screenshots
Basic flow:
Simulator.Screen.Recording.-.iPhone.16.-.2025-08-07.at.15.43.21.mp4
Edge case for JCP sites when logged with wrong account:
Simulator.Screen.Recording.-.iPhone.16.-.2025-08-08.at.15.03.39.mp4
RELEASE-NOTES.txtif necessary.